Add `experimentalForceOwningTab` for Web Worker support #2197

thebrianchen · 2019-09-26T21:48:39Z

Original issue: #983
Provides an experimental feature to support offline persistence in web workers.

I've updated the sample app with the latest changes as well (link).

There also seem to be some changes from running prettier that probably weren't pushed in other commits.

mikelehen

Mostly LGTM but I left a few nits and I think we need to refine when force can override canActAsPrimary...

packages/firebase/index.d.ts

mikelehen · 2019-09-27T18:43:34Z

packages/firebase/index.d.ts

+     * Whether to force enable persistence for the client. This cannot be used
+     * with `synchronizeTabs:true` and is primarily intended for use with Web
+     * Workers. Setting this to 'true' will enable persistence, but cause other
+     * tabs using persistence to fail.


note: If/when we ship this for real we should add more detail about exactly how other tabs will fail (so that developers can potentially write code to detect it, etc.).

packages/firestore/src/core/firestore_client.ts

packages/firestore/src/local/indexeddb_persistence.ts

packages/firestore/src/local/simple_db.ts

mikelehen · 2019-09-27T19:26:39Z

packages/firestore/src/local/simple_db.ts

+      if (indexedDB !== undefined) {
+        request = indexedDB.open(name, version);
+      } else {
+        request = window.indexedDB.open(name, version);


Since window is usually the global object and we've already checked for the indexedDB global directly, is there a case where we will ever hit this?

Maybe we have a fake window object in node tests or something? Do you know?

I demand answers too :)

Looked into this some more and since window is bound to globalThis in the dom, we won't ever hit this line unless window has been mocked out and the indexedDB global doesn't exist.

Deleted.

mikelehen · 2019-09-27T19:27:14Z

packages/firestore/src/local/simple_db.ts

+    // If IndexedDb is available directly (ex: in case of web workers).
+    if (typeof indexedDB !== 'undefined') {
+      return true;
+    }
    if (typeof window === 'undefined' || window.indexedDB == null) {


Per above, since window is usually the global object and we've already checked for the indexedDB global directly, is there a case where we will ever hit this?

We still need this for cases where indexedDB and window aren't available and return false, or else an environment without a global indexedDB that passes the other checks will incorrectly return true for isAvailable().

I think we just need to register the IndexedDb shim directly on globalAny in node_persistence.ts.

Added comment for why it's already registered.

Doesn't that mean that "if (typeof window === 'undefined' || window.indexedDB == null)" is now no longer needed? Is your distinction from your first response here still relevant? if so, we should probably talk this through offline.

As per offline conversation, removed the window logic, and flipped the indexedDB check to return false. By returning true automatically when IndexedDB is available, we are skipping all of the UA checks.

Also removed the window.navigator check, to make sure workers aren't rejected early.

mikelehen

This looks good for the purpose of gathering feedback!

thebrianchen

thanks for sticking through with me on this :)

thebrianchen · 2020-05-19T19:32:25Z

packages/firestore/test/unit/local/indexeddb_persistence.test.ts

+    return withPersistence('clientA', async db => {
+      await withForcedPersistence('clientB', async () => {
+        return expect(
+          db.runTransaction('tx', 'readwrite-primary', () =>


thebrianchen · 2020-05-19T19:33:56Z

packages/firestore/src/local/indexeddb_persistence.ts

@@ -1005,11 +1024,12 @@ export class IndexedDbPersistence implements Persistence {

  private detachWindowUnloadHook(): void {
    if (this.windowUnloadHandler) {
+      assert(this.window != null, "'window' should be defined");


Didn't realized we've separated out asserts. thanks

thebrianchen · 2020-05-19T19:35:56Z

packages/firestore/src/local/indexeddb_persistence.ts

@@ -239,7 +242,8 @@ export class IndexedDbPersistence implements Persistence {
    lruParams: LruParams,
    private readonly queue: AsyncQueue,
    serializer: JsonProtoSerializer,
-    private readonly sequenceNumberSyncer: SequenceNumberSyncer
+    private readonly sequenceNumberSyncer: SequenceNumberSyncer,
+    private readonly forceOwningTab: boolean


thebrianchen · 2020-05-19T22:53:09Z

packages/firestore/src/local/indexeddb_persistence.ts

@@ -587,6 +602,9 @@ export class IndexedDbPersistence implements Persistence {
          }

          if (!this.isLocalClient(currentPrimary)) {
+            if (this.forceOwningTab) {


Since we are already using the forceOwningTab logic in updateClientMetadataAndTryBecomePrimary, we don't need the check here anymore. Removed.

thebrianchen · 2020-05-19T23:00:14Z

packages/firestore/src/local/simple_db.ts

+    // If IndexedDb is available directly (ex: in case of web workers).
+    if (typeof indexedDB !== 'undefined') {
+      return true;
+    }
    if (typeof window === 'undefined' || window.indexedDB == null) {


As per offline conversation, removed the window logic, and flipped the indexedDB check to return false. By returning true automatically when IndexedDB is available, we are skipping all of the UA checks.

Also removed the window.navigator check, to make sure workers aren't rejected early.

thebrianchen · 2020-05-20T05:54:07Z

packages/firestore/src/local/indexeddb_persistence.ts

-        'IndexedDB persistence is only available on platforms that support LocalStorage.'
-      );
+      this.webStorage = null;
+      if (forceOwningTab === false) {


We now throw an error if synchronizeTabs is used with experimentalForceOwningTab, and we throw an error if LocalStorage is unavailable in WebStorageSharedClientState. Which case are we not covering?

…lientMetadataAndTryBecomePrimaryc

schmidt-sebastian

LGTM

schmidt-sebastian · 2020-05-27T16:52:43Z

packages/firestore/src/local/indexeddb_persistence.ts

@@ -585,6 +585,9 @@ export class IndexedDbPersistence implements Persistence {
  private canActAsPrimary(
    txn: PersistenceTransaction
  ): PersistencePromise<boolean> {
+    if (this.forceOwningTab) {
+      return PersistencePromise.resolve<boolean>(true);


You can probably drop the generic since it can be inferred.

Not sure why TS can't infer it, but the error I get is:
Type 'PersistencePromise<true>' is not assignable to type 'PersistencePromise<boolean>'.

egilmorez

All language in index.d.ts LGTM with a tiny nit. Thanks!

egilmorez · 2020-06-01T18:31:20Z

packages/firebase/index.d.ts

+    /**
+     * Whether to force enable persistence for the client. This cannot be used
+     * with `synchronizeTabs:true` and is primarily intended for use with Web
+     * Workers. Setting this to 'true' will enable persistence, but cause other


Was the intention to use backticks around true ? That would work.

Feiyang1 · 2020-06-01T22:13:51Z

config/functions/project.json

@@ -0,0 +1,9 @@
+ {


Do you mean to commit this file?

thanks for catching

thebrianchen added the api: firestore label Sep 26, 2019

thebrianchen requested a review from mikelehen September 26, 2019 21:48

thebrianchen requested review from Feiyang1, hiranya911 and hsubox76 as code owners September 26, 2019 21:48

thebrianchen assigned mikelehen Sep 26, 2019

cleaned up

f0287d4

thebrianchen force-pushed the bc/ww-persist branch from 4ede337 to f0287d4 Compare September 27, 2019 16:23

Brian Chen added 2 commits September 27, 2019 09:59

fix tests

15cf32e

remove comments

66655d0

mikelehen suggested changes Sep 27, 2019

View reviewed changes

mikelehen assigned thebrianchen and unassigned mikelehen Sep 27, 2019

fix michael comments

9ac6e2d

thebrianchen requested a review from mikelehen September 30, 2019 21:31

thebrianchen assigned mikelehen and unassigned thebrianchen Sep 30, 2019

mikelehen reviewed Oct 1, 2019

View reviewed changes

mikelehen assigned thebrianchen and unassigned mikelehen Oct 1, 2019

Brian Chen added 6 commits October 21, 2019 16:50

Merge branch 'master' into bc/ww-persist

d813905

Merge branch 'master' into bc/ww-persist

347b722

[AUTOMATED]: Prettier Code Styling

91404e3

Merge branch 'release' into bc/ww-persist

00c6dc7

Merge branch 'master' into bc/ww-persist

6dc5e51

Merge branch 'master' into bc/ww-persist

fef8971

thebrianchen requested a review from egilmorez as a code owner February 13, 2020 21:54

Brian Chen added 3 commits May 7, 2020 11:04

Merge branch 'master' into bc/ww-persist

8366646

update post API council

741e88e

[AUTOMATED]: Prettier Code Styling

86ca4aa

schmidt-sebastian assigned thebrianchen and unassigned schmidt-sebastian May 19, 2020

update isAvailable() logic + comments round3

7d8e41a

thebrianchen commented May 20, 2020

View reviewed changes

add forceOwningTab logic back to canActAsPrimary, remove from updateC…

6d20b20

…lientMetadataAndTryBecomePrimaryc

thebrianchen requested review from schmidt-sebastian May 26, 2020 21:48

thebrianchen assigned schmidt-sebastian and unassigned thebrianchen May 26, 2020

schmidt-sebastian assigned thebrianchen and unassigned schmidt-sebastian May 26, 2020

Brian Chen added 2 commits May 26, 2020 18:30

update forceOwningTab comment

016aed8

move forceOwningTab logic up in canActAsPrimary

31a8772

schmidt-sebastian approved these changes May 27, 2020

View reviewed changes

Brian Chen added 3 commits May 27, 2020 22:30

Merge branch 'master' into bc/ww-persist

c7cbf9a

update new test from master

be3a0d4

Merge branch 'master' into bc/ww-persist

47cd1f6

thebrianchen assigned egilmorez and unassigned thebrianchen May 28, 2020

sesam mentioned this pull request May 31, 2020

Need feedback for changes to version 1.0.0 samuelgozi/firebase-firestore-lite#21

Closed

egilmorez approved these changes Jun 1, 2020

View reviewed changes

s/'true'/

0f45287

thebrianchen assigned Feiyang1 and unassigned egilmorez Jun 1, 2020

Feiyang1 reviewed Jun 1, 2020

View reviewed changes

remove project.json

9cfe34d

Feiyang1 approved these changes Jun 1, 2020

View reviewed changes

thebrianchen merged commit 5547861 into master Jun 1, 2020

thebrianchen deleted the bc/ww-persist branch June 1, 2020 23:52

firebase locked and limited conversation to collaborators Jul 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `experimentalForceOwningTab` for Web Worker support #2197

Add `experimentalForceOwningTab` for Web Worker support #2197

thebrianchen commented Sep 26, 2019 •

edited

Loading

mikelehen left a comment

mikelehen Sep 27, 2019

mikelehen Sep 27, 2019

schmidt-sebastian May 8, 2020

thebrianchen May 13, 2020

mikelehen Sep 27, 2019

schmidt-sebastian May 8, 2020

thebrianchen May 13, 2020

schmidt-sebastian May 14, 2020

thebrianchen May 18, 2020

schmidt-sebastian May 19, 2020

thebrianchen May 19, 2020

mikelehen left a comment

thebrianchen left a comment

thebrianchen May 19, 2020

thebrianchen May 19, 2020

thebrianchen May 19, 2020

thebrianchen May 19, 2020

thebrianchen May 19, 2020

thebrianchen May 20, 2020

schmidt-sebastian left a comment

schmidt-sebastian May 27, 2020

thebrianchen May 28, 2020

egilmorez left a comment

egilmorez Jun 1, 2020

thebrianchen Jun 1, 2020

Feiyang1 Jun 1, 2020

thebrianchen Jun 1, 2020

Add experimentalForceOwningTab for Web Worker support #2197

Add experimentalForceOwningTab for Web Worker support #2197

Conversation

thebrianchen commented Sep 26, 2019 • edited Loading

mikelehen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikelehen left a comment

Choose a reason for hiding this comment

thebrianchen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

egilmorez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `experimentalForceOwningTab` for Web Worker support #2197

Add `experimentalForceOwningTab` for Web Worker support #2197

thebrianchen commented Sep 26, 2019 •

edited

Loading